-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(tools): bootstrap nx workpsace-tools and create workspace generator #18194
chore(tools): bootstrap nx workpsace-tools and create workspace generator #18194
Conversation
45a0722
to
a82fe97
Compare
"@nrwl/jest": "12.1.0", | ||
"@nrwl/node": "12.1.0", | ||
"@nrwl/tao": "12.1.0", | ||
"@nrwl/workspace": "12.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reordered by yarn add
new installed packages:
"@nrwl/devkit": "12.1.0",
"@nrwl/jest": "12.1.0",
"@nrwl/node": "12.1.0",
tslib": "2.2.0 --> added to root (single version policy)
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 975140887c6ad2391f8db63003ed440239ec9c25 (build) |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit e8f7adc:
|
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 972 | 970 | 5000 | |
BaseButton | mount | 995 | 1052 | 5000 | |
Breadcrumb | mount | 2798 | 2826 | 1000 | |
ButtonNext | mount | 571 | 586 | 5000 | |
Checkbox | mount | 1820 | 1775 | 5000 | |
CheckboxBase | mount | 1478 | 1467 | 5000 | |
ChoiceGroup | mount | 5406 | 5350 | 5000 | |
ComboBox | mount | 1081 | 1110 | 1000 | |
CommandBar | mount | 11066 | 11097 | 1000 | |
ContextualMenu | mount | 6794 | 6811 | 1000 | |
DefaultButton | mount | 1291 | 1304 | 5000 | |
DetailsRow | mount | 4231 | 4179 | 5000 | |
DetailsRowFast | mount | 4151 | 4195 | 5000 | |
DetailsRowNoStyles | mount | 3960 | 3881 | 5000 | |
Dialog | mount | 2334 | 2302 | 1000 | |
DocumentCardTitle | mount | 164 | 161 | 1000 | |
Dropdown | mount | 3636 | 3658 | 5000 | |
FocusTrapZone | mount | 1927 | 1955 | 5000 | |
FocusZone | mount | 1921 | 1901 | 5000 | |
IconButton | mount | 1963 | 1979 | 5000 | |
Label | mount | 368 | 377 | 5000 | |
Layer | mount | 2042 | 2024 | 5000 | |
Link | mount | 536 | 522 | 5000 | |
MakeStyles | mount | 1959 | 1943 | 50000 | |
MenuButton | mount | 1707 | 1689 | 5000 | |
MessageBar | mount | 2232 | 2248 | 5000 | |
Nav | mount | 3631 | 3698 | 1000 | |
OverflowSet | mount | 1145 | 1104 | 5000 | |
Panel | mount | 2243 | 2240 | 1000 | |
Persona | mount | 913 | 916 | 1000 | |
Pivot | mount | 1571 | 1592 | 1000 | |
PrimaryButton | mount | 1480 | 1491 | 5000 | |
Rating | mount | 8674 | 8814 | 5000 | |
SearchBox | mount | 1565 | 1553 | 5000 | |
Shimmer | mount | 2890 | 2912 | 5000 | |
Slider | mount | 2200 | 2219 | 5000 | |
SpinButton | mount | 5519 | 5593 | 5000 | |
Spinner | mount | 453 | 468 | 5000 | |
SplitButton | mount | 3584 | 3587 | 5000 | |
Stack | mount | 568 | 558 | 5000 | |
StackWithIntrinsicChildren | mount | 1673 | 1719 | 5000 | |
StackWithTextChildren | mount | 5252 | 5127 | 5000 | |
SwatchColorPicker | mount | 11418 | 11285 | 5000 | |
Tabs | mount | 1572 | 1583 | 1000 | |
TagPicker | mount | 2675 | 2759 | 5000 | |
TeachingBubble | mount | 12757 | 12829 | 5000 | |
Text | mount | 447 | 480 | 5000 | |
TextField | mount | 1546 | 1646 | 5000 | |
ThemeProvider | mount | 1278 | 1310 | 5000 | |
ThemeProvider | virtual-rerender | 638 | 630 | 5000 | |
ThemeProviderNext | mount | 7011 | 7056 | 5000 | |
Toggle | mount | 908 | 914 | 5000 | |
buttonNative | mount | 115 | 128 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
SegmentMinimalPerf.default | 448 | 401 | 1.12:1 |
AvatarMinimalPerf.default | 230 | 208 | 1.11:1 |
SkeletonMinimalPerf.default | 435 | 403 | 1.08:1 |
TreeWith60ListItems.default | 205 | 189 | 1.08:1 |
RefMinimalPerf.default | 269 | 252 | 1.07:1 |
GridMinimalPerf.default | 399 | 377 | 1.06:1 |
ListCommonPerf.default | 751 | 711 | 1.06:1 |
TextMinimalPerf.default | 409 | 386 | 1.06:1 |
VideoMinimalPerf.default | 734 | 692 | 1.06:1 |
BoxMinimalPerf.default | 410 | 390 | 1.05:1 |
HeaderMinimalPerf.default | 430 | 411 | 1.05:1 |
InputMinimalPerf.default | 1409 | 1344 | 1.05:1 |
MenuButtonMinimalPerf.default | 1848 | 1756 | 1.05:1 |
AttachmentSlotsPerf.default | 1317 | 1270 | 1.04:1 |
HeaderSlotsPerf.default | 919 | 884 | 1.04:1 |
ListMinimalPerf.default | 572 | 552 | 1.04:1 |
AlertMinimalPerf.default | 326 | 315 | 1.03:1 |
ButtonMinimalPerf.default | 195 | 189 | 1.03:1 |
FlexMinimalPerf.default | 330 | 321 | 1.03:1 |
ImageMinimalPerf.default | 435 | 424 | 1.03:1 |
LabelMinimalPerf.default | 454 | 441 | 1.03:1 |
ListWith60ListItems.default | 742 | 723 | 1.03:1 |
ProviderMinimalPerf.default | 1066 | 1038 | 1.03:1 |
StatusMinimalPerf.default | 809 | 785 | 1.03:1 |
TableManyItemsPerf.default | 2230 | 2163 | 1.03:1 |
TreeMinimalPerf.default | 913 | 884 | 1.03:1 |
AnimationMinimalPerf.default | 463 | 452 | 1.02:1 |
CheckboxMinimalPerf.default | 3014 | 2968 | 1.02:1 |
ItemLayoutMinimalPerf.default | 1444 | 1409 | 1.02:1 |
LayoutMinimalPerf.default | 425 | 415 | 1.02:1 |
PopupMinimalPerf.default | 630 | 619 | 1.02:1 |
PortalMinimalPerf.default | 182 | 178 | 1.02:1 |
ProviderMergeThemesPerf.default | 1772 | 1738 | 1.02:1 |
CardMinimalPerf.default | 643 | 637 | 1.01:1 |
ChatMinimalPerf.default | 701 | 695 | 1.01:1 |
EmbedMinimalPerf.default | 4539 | 4506 | 1.01:1 |
FormMinimalPerf.default | 480 | 475 | 1.01:1 |
RadioGroupMinimalPerf.default | 497 | 492 | 1.01:1 |
SplitButtonMinimalPerf.default | 4222 | 4161 | 1.01:1 |
CustomToolbarPrototype.default | 4143 | 4100 | 1.01:1 |
ButtonOverridesMissPerf.default | 1821 | 1822 | 1:1 |
ButtonSlotsPerf.default | 622 | 622 | 1:1 |
CarouselMinimalPerf.default | 531 | 531 | 1:1 |
DialogMinimalPerf.default | 830 | 831 | 1:1 |
DropdownManyItemsPerf.default | 772 | 774 | 1:1 |
ListNestedPerf.default | 659 | 660 | 1:1 |
MenuMinimalPerf.default | 960 | 960 | 1:1 |
IconMinimalPerf.default | 714 | 713 | 1:1 |
TextAreaMinimalPerf.default | 568 | 569 | 1:1 |
TooltipMinimalPerf.default | 1091 | 1090 | 1:1 |
DatepickerMinimalPerf.default | 6109 | 6153 | 0.99:1 |
ToolbarMinimalPerf.default | 1052 | 1062 | 0.99:1 |
DividerMinimalPerf.default | 405 | 413 | 0.98:1 |
DropdownMinimalPerf.default | 3277 | 3328 | 0.98:1 |
LoaderMinimalPerf.default | 773 | 786 | 0.98:1 |
SliderMinimalPerf.default | 1644 | 1670 | 0.98:1 |
TableMinimalPerf.default | 462 | 473 | 0.98:1 |
ChatDuplicateMessagesPerf.default | 335 | 347 | 0.97:1 |
ReactionMinimalPerf.default | 430 | 442 | 0.97:1 |
AttachmentMinimalPerf.default | 187 | 197 | 0.95:1 |
RosterPerf.default | 1270 | 1359 | 0.93:1 |
ChatWithPopoverPerf.default | 387 | 422 | 0.92:1 |
AccordionMinimalPerf.default | 158 | 186 | 0.85:1 |
8c2119c
to
7302563
Compare
"@nrwl/workspace": "12.1.0", | ||
"@nrwl/cli": "12.1.0", | ||
"@nrwl/tao": "12.1.0" | ||
"webpack-bundle-analyzer": "4.4.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagated packages from scripts/ to single version policy as they had chalk as a dep which was causing issues
"just-scripts": "1.3.1",
"webpack-dev-middleware": "4.2.0",
"webpack-hot-middleware": "2.25.0",
"beachball": "1.53.1", | ||
"chalk": "4.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bumped chalk to 4.1.0
-> after I added additional @nrwl/*
packages (they have chalk 4.1 as dependency),
build started to fail
-> because react-conformance still depends on old chalk 2 ( which did lot of mistakes regarding exposing APIs for js and ts consumers), so scripts/exec
was loading chalk 4.1 but react-conformance
was supposed to consume 2.x but because you cannot consume 2 versions of the same package without hacks (aliasing), react-conformance got v4 which has different API and thus failed build
"ts-jest": "24.3.0", | ||
"ts-loader": "8.0.14", | ||
"tsconfig-paths-webpack-plugin": "3.5.1", | ||
"tslib": "2.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
propagated tslib to single version policy for devtooling
package.json
Outdated
@@ -176,7 +184,7 @@ | |||
}, | |||
"syncpack": { | |||
"prod": true, | |||
"dev": true, | |||
"dev": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I turned off checks for devDeps. This tool wont work with what we agreed upon
-> single version policy === exact versions
-> published package deps === caret versions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is a good idea until we have complete single version policy for dev deps (and tooling to enforce it). Instead I wonder if you could add a versionGroups
entry something like this:
{
"packages": ["fluent-ui-react-repo"],
"dependencies": ["tslib", ...]
}
(If there's some other syncpack setting we need to support this scenario longer-term, the syncpack author has been pretty responsive to suggestions.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this works:
{
"packages": [
"fluent-ui-react-repo"
],
"dependencies": [
"tslib",
"webpack-bundle-analyzer",
"chalk"
]
},
although I don't think this is what we want, as it will allow further misalignments between versions.
What would be probably needed as a feature for syncpack is the ability to specify devDependencies similarly to how dependencies work.
@@ -1,7 +1,7 @@ | |||
// @ts-check | |||
const path = require('path'); | |||
const child_process = require('child_process'); | |||
const chalk = require('chalk').default; | |||
const chalk = require('chalk'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
chalk v4 api
@@ -32,6 +32,10 @@ jobs: | |||
yarn checkchange | |||
displayName: check change | |||
|
|||
- script: | | |||
yarn nx workspace-lint | |||
displayName: nx:workspace-lint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run nx workspace lint checks on ci to prevent getting out of sync whilst doing changes by hand
}, | ||
"include": [], | ||
"files": [], | ||
"references": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
environment scoped TS config - this is the future for every convergence package (will mitigate issues like this -> #17101)
@@ -0,0 +1,13 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is only one special case for nx, as it compiles down TS source of workspace generators - it uses this file in particular
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still a bit confused by intention of both tsconfig.tools.json
and tsconfig.lib.json
, why are both necessary ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry about that.
tsconfig.tools.json
is used to runtsc
when executingnx workspace-generator <generator-name>
runnertsconfig.lib.json
is standard part of every package ( 100% converged to nx ), which is used for production builds (transpile/rollup) and as source of truth for type checking implementation files.- I added it to
/tools
to keep the default boilerplate consistent
- I added it to
@@ -472,6 +468,33 @@ | |||
"@fluentui/react-popover": { | |||
"root": "packages/react-popover", | |||
"projectType": "library" | |||
}, | |||
"nx-workspace-tools": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a showcase how task managed will look like in the future with full nx adoption. for now, it's leveraged only for tools/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we be duplicating this kind of json config for all packages listed in the workspace.json
in the future ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will we be duplicating this kind of json config for all packages listed in the workspace.json in the future ?
we might, yes -> meaning no more package.json#scripts. It gives us various benefits over primitive npm scripts, integration with vscode plugin etc.
spoiler alert -> nx 13 will add new features that this huge workspace json will be dissected into smaller ones per package
{ | ||
"extends": ["plugin:@fluentui/eslint-plugin/node"], | ||
"rules": { | ||
"import/no-extraneous-dependencies": "off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess anything in tools
will not declare dependencies in package.json
and just consume from the root right ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, single version policy from the start.
- also because this will never be published
- there is no need to provide deps
- if we might change our minds, nx generates package.json with deps automatically (based on AST/dependency graph)
@@ -0,0 +1,10 @@ | |||
export interface WorkspaceGeneratorGeneratorSchema { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this generated from the schema.json
or maintained manually ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
generated. we can add that tooling later.
throw new Error('name is required'); | ||
} | ||
|
||
const defaults = { skipFormat: false }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this default value is declared in schema.json
, does this mean nx
does not handle default values declared in the schema ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this default value is declared in schema.json, does this mean nx does not handle default values declared in the schema ?
- it does, typescript doesn't - the ultimate problem of default vs optional values.
JSX has special hack for that in TS compiler (via *.defaultProps
), outside of that we are out of luck. I mean it can be hacked via additional TS type transforms which adds additional churn, from my experience this is the most concise way. If you have other proposal how to handle this lemme know!
The "churn" I mentioned:
import { WorkspaceGeneratorGeneratorSchema } from './schema';
// while this looks "ok" for 1 property, for more it might become hard to read ? (YMMV)
interface SchemaWithAccommodatedDefaults extends Required<Pick<WorkspaceGeneratorGeneratorSchema,'skipFormat'>>,WorkspaceGeneratorGeneratorSchema {}
export default async function(host: Tree, schema: SchemaWithAccommodatedDefaults) {
// ....
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving this since it sounded like you needed this merged for some work today, but it would be helpful if you could go over the PR in a meeting (maybe the Thursday sync) and explain a bit more about the new setup.
package.json
Outdated
@@ -176,7 +184,7 @@ | |||
}, | |||
"syncpack": { | |||
"prod": true, | |||
"dev": true, | |||
"dev": false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this is a good idea until we have complete single version policy for dev deps (and tooling to enforce it). Instead I wonder if you could add a versionGroups
entry something like this:
{
"packages": ["fluent-ui-react-repo"],
"dependencies": ["tslib", ...]
}
(If there's some other syncpack setting we need to support this scenario longer-term, the syncpack author has been pretty responsive to suggestions.)
{ | ||
"extends": ["plugin:@fluentui/eslint-plugin/node"], | ||
"rules": { | ||
"import/no-extraneous-dependencies": "off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for turning this off? Was the config not properly allowing deps from the root config (there's a setting for that), or some other issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works for me locally:
"import/no-extraneous-dependencies": ["error", { "packageDir": [".", ".."] }]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually never mind, that works in the UI but not in the tool. To make it work while running yarn lint
it has to be "packageDir": ["."]
--I guess nx run
runs stuff with cwd
as the repo root?
"compilerOptions": { | ||
"module": "commonjs", | ||
"target": "ES2015", | ||
"outDir": "../dist/out-tsc", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we ever made a decision about whether to put the output in a single dist
folder at the root. Unless it's necessary for nx to have it at the root for some reason, could it stay inside the tools
folder for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless it's necessary for nx to have it at the root for some reason, could it stay inside the tools folder for now
- right, this is default nx boilerplate. In general every build output is stored within root
dist/
->/dist/<package-name>/....
.- a) this is required for TS when path aliases are used ( currently we "hack it" by removing aliases during lage builds)
- b) I mentioned reasoning in this RFC
- in reality within /tools, this config file is solely for proper type checking per environment ( in this case,
lib
-> implementation code )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock future work, this doesn't impact any of our shipped code and will become clearer once it does.
yup, that was my plan. We might extend the meeting time because of this. |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
@Hotell When I try to run
(Edit: changing |
Pull request checklist
$ yarn change
Description of changes
this PR addresses 2 things
1. bumping/resolving issues with chalk
2. adds nx workspace-tools capabilities
/tools
"project" which is a default nx tooling root for authoring workspace related generators and executorsworkspace-generator
generator which will be used as basic building block for bootstraping workspace related generators/migrations (adds proper testing setup and schematic interface boilerplate, which is missing in default nx workspace-generator )About: Running workspace generators
yarn nx workspace-generator [generator-name]
↓↓↓↓
↓↓↓↓
Focus areas to test
(optional)